[SCFD-2744] [SCFD-2069] Added BC completeness check #489
+530
−45
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benflexcompute
changed the title
benflexcompute
changed the title
| @@ -185,7 +185,7 @@ def decorator(func: Callable): | |||
| def wrapper(self: Any, *args, **kwargs): | |||
| current_levels = get_validation_levels() | |||
| # Run the validator only if the current levels matches the specified context or is ALL | |||
| if current_levels is None or any(lvl in (context, ALL) for lvl in current_levels): | |||
| if current_levels is not None and any(lvl in (context, ALL) for lvl in current_levels): | |||
There was a problem hiding this comment.
that was not mistake. The motivation was the following:
if user is constructing SimulationParams and uses any parameter, we always validate this parameter. It is most likely optional or belongs to optional parent. User is free to remove it to pass validation. It is context irrelevant that any explicit inputs must be valid.
Thus we always run validator when running SimulationParams without validation context. But for developer user, for example web GUI integration or some python client functionalities, we can ignore some errors as we understand why some errors can be ignored. That is the motivation of this logic here.
There was a problem hiding this comment.
But then what is the point of context_validator other than providing the context of error? Or is this exactly the purpose?
There was a problem hiding this comment.
that is the purpose
| param_boundary_registry.register(surface_pair.pair[1]) | ||
|
|
||
| for boundary in asset_boundaries: | ||
| if param_boundary_registry.contains(boundary) is False: |
There was a problem hiding this comment.
.contain seems be linear and this is also linear loop, which makes it quadratic complexity. .contain at least can be improved to be O(log(N)).
Overall I think we can do 2 checks to improve performance here:
- check for duplication, O(N)
- check for array equality, we can use Counter as items are hashable and that would be O(N)
so for most checks we will have O(N) cost
only if the above checks fail, we run O(N*log(N)) check to indicate which boundaries are not set or set multiple times.
There was a problem hiding this comment.
I am confused. Can we have a meeting tomorrow?
There was a problem hiding this comment.
Using set to speed things up. Checking the length of the two sets does not really help since there might be different items in two sets.
Comment on lines
280
to
281
| if len(params.models) == 1 and isinstance(params.models[0], Fluid): | ||
| return params |
There was a problem hiding this comment.
if there are no BC defined at all, why don't we raise?
There was a problem hiding this comment.
because even if user composes a SimulationParams for meshing only this validator will still get triggered. The context validator does not control when validator is called.
There was a problem hiding this comment.
Changed this validator to adapt with current validation context. We only raise if we have CASE ValidationContext
flow360/component/simulation/validation/validation_simulation_params.py
Outdated
Show resolved
Hide resolved
…params.py Co-authored-by: Maciej Skarysz <83596707+maciej-flexcompute@users.noreply.github.com>
…params.py Co-authored-by: Maciej Skarysz <83596707+maciej-flexcompute@users.noreply.github.com>
maciej-flexcompute
approved these changes
Nov 13, 2024
benflexcompute
added a commit
that referenced
this pull request
Nov 14, 2024
* added 0m as default inner radius (#542) * Fix wrong item type for forking a case (#544) * Add from_local_storage function for creating geometry instance with local geometry file (#554) * Add from_local_storage function for geometry. * Add new TimeAverage output to new_simulation_model.py * Fix Formatting * Address comments * Address comment * fix(): windows will use \ as path separator, but aws s3 only accept / as path separator (#551) Co-authored-by: Maciej Skarysz <83596707+maciej-flexcompute@users.noreply.github.com> * [SCFD-3093] added support for user defined Fairfield (#558) * added support for user defined farfield * Fix lint * Fixed rotation zone tranlsation (#557) * Added user defined fields support for V2.0 Flow360 (#560) * Added user defined fields support for V2.0 Flow360 * Fix unit tests * Comments addressed * Fix unit test (#563) * added support for x slicing force distribution and y slicing force distribution (#561) * added support for x slicing force distribution and y slicing force distribution * fixed isort * added missing data * adderessed PR comments --------- Co-authored-by: Ben <106089368+benflexcompute@users.noreply.github.com> * Fixed when setting param's entity_info, the tags are not included (#552) * Added validation that `parent_volume` also needs to be rotating (#566) * Added validaiton that parent_volume also needs to be rotating * Removed print * Fixed the UDD translation by removing the output target (#567) * Beta 24.11 (#570) * added CI for the branch * Flow360 2.0 documentation for volume_models, solver_numerics and outputs modules (#519) * Test edit on VolumeOutput * Re-commit documentation change * Address comments --------- Co-authored-by: Ben <106089368+benflexcompute@users.noreply.github.com> * Grouping V1 python client components so they are not exposed directly to user. (#529) * Grouped V1 unit tests and removed unused fixtures * Added missing files * moved flow360_params folder as v1 folder * added flow360/component/constants.py back * Fixed unit tests * Moved previous root level modules to v1 * Fixed bug in error message * Fixed circular import * Fix lint * Fixed unit tests * Fixed unit test * Fix lint * rm new_simulation_models.py since it is copied as __init__.py * comments addressed * Added back gitignore for mesh files since we do not need them in the repo (they come from cloud) * Fixed linting * Deleting cgns * deleted the new_simulation_models.py which is repalced by __init__.py * Rename `__init__.py` from v1 folder as modules.py (#541) * Lint fixed. now need to fix unit system conflict * Fixed unit system conflict by renaming it * Moved modules.py to v1.py * comment addressed * Add documentation for after reorganization (#543) * Add sphinx params plugins * Add more documentations * Update Flow360 * Update solver_numerics * Fix isort --------- Co-authored-by: Ben <106089368+benflexcompute@users.noreply.github.com> * Ben y/add missing sphinx package (#545) * Added sphinx-paramlinks * Add more dependencies * Update Isosurface related docs (#546) * Ben y/add meshing and exmp (#547) * WIP-1 * WIP * Fix lint * Fixed unit test * Fix unit test * Added more examples * fix(): windows will use \ as path separator, but aws s3 only accept / as path separator (#551) Co-authored-by: Maciej Skarysz <83596707+maciej-flexcompute@users.noreply.github.com> * Update flow360/component/simulation/models/surface_models.py Co-authored-by: yifan-flex <124317394+yifan-flex@users.noreply.github.com> * Update flow360/component/simulation/models/surface_models.py Co-authored-by: yifan-flex <124317394+yifan-flex@users.noreply.github.com> * Add documentation for operating condition (#556) * Add documentation for operating condition * Address PR comments * Removed the blank line that cause doc confusion (#564) * Update init for documentation (#568) * Sphinx warning fixes * Add operating condition function * Fix sphinx warnings * Minor fix --------- Co-authored-by: angranl-flex <angran@flexcompute.com> Co-authored-by: JunchengXue <97111055+JunchengXue@users.noreply.github.com> Co-authored-by: Maciej Skarysz <83596707+maciej-flexcompute@users.noreply.github.com> Co-authored-by: yifan-flex <124317394+yifan-flex@users.noreply.github.com> * [SCFD-3148] Added private_attribute_id for all entities (#550) * Added private_attribute_id for all entities to ensure that front end entity tracking still works * Testing through deploying * Fixed unit test * Fixed unit test and removed unnecessary changes * Fix linting * Commments addressed * Fix UDD class (#571) * Replaced str with StringExpression (#569) * Replaced str with StringExpression * Updated a more comprehensive list * [SCFD-2744] [SCFD-2069] Added BC completeness check (#489) * Added BC completeness check * Fixing unit test * Fixed unit test * Fix lint * Update flow360/component/simulation/validation/validation_simulation_params.py Co-authored-by: Maciej Skarysz <83596707+maciej-flexcompute@users.noreply.github.com> * Update flow360/component/simulation/validation/validation_simulation_params.py Co-authored-by: Maciej Skarysz <83596707+maciej-flexcompute@users.noreply.github.com> * Fixed the unit test * FIX UNIT TEST * Added guard on the validation level --------- Co-authored-by: Maciej Skarysz <83596707+maciej-flexcompute@users.noreply.github.com> * Fix Transition Solver Field and Add Conflict Fields Unit Test (#574) * Fix Transition Solver Field and add conflict fields unit test * Add validators for TransitionModelSover * Update function name * Formatting * [SCFD-3113] Added validation that vector should not contain None components (#515) * [SCFD-3113] Added validation that vector should not contain None components * Moved the implementation into a dedicated validator * Fixed unit test * Update flow360/component/simulation/unit_system.py Co-authored-by: Maciej Skarysz <83596707+maciej-flexcompute@users.noreply.github.com> --------- Co-authored-by: Maciej Skarysz <83596707+maciej-flexcompute@users.noreply.github.com> * Prepare for 24.11.0b1 release * update version for b2 release * [SCFD-2695] Reworked the manipulate restart mechanism (#575) * Reworked the manipulate restart mechanisim * Fix format --------- Co-authored-by: Maciej Skarysz <83596707+maciej-flexcompute@users.noreply.github.com> Co-authored-by: angranl-flex <angran@flexcompute.com> Co-authored-by: JunchengXue <97111055+JunchengXue@users.noreply.github.com> Co-authored-by: yifan-flex <124317394+yifan-flex@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.